Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MNG-8286] Add a condition profile based on a simple expressions #1771

Merged
merged 2 commits into from
Nov 20, 2024

Conversation

gnodet
Copy link
Contributor

@gnodet gnodet commented Oct 3, 2024

JIRA issue MNG-8286

In addition to the traditional activation mechanisms (JDK version, OS properties, file existence, etc.), Maven now supports a powerful condition-based activation through the condition field. This new mechanism allows for more flexible and expressive profile activation rules.

Condition Syntax

The condition is specified as a string expression that can include various functions, comparisons, and logical operators.
Some key features include:

  • Property access: ${property.name}
  • Comparison operators: ==, !=, <, >, <=, >=
  • Logical operators: && (AND), || (OR), not(...)
  • Functions: exists(...), missing(...), matches(...), inrange(...), and more

Supported Functions

The following functions are supported in condition expressions:

  • length(string): Returns the length of the given string.
  • upper(string): Converts the string to uppercase.
  • lower(string): Converts the string to lowercase.
  • substring(string, start, [end]): Returns a substring of the given string.
  • indexOf(string, substring): Returns the index of the first occurrence of substring in string, or -1 if not found.
  • contains(string, substring): Checks if the string contains the substring.
  • matches(string, regex): Checks if the string matches the given regular expression.
  • not(condition): Negates the given condition.
  • if(condition, trueValue, falseValue): Returns trueValue if the condition is true, falseValue otherwise.
  • exists(path): Checks if a file matching the given glob pattern exists.
  • missing(path): Checks if a file matching the given glob pattern does not exist.
  • inrange(version, range): Checks if the given version is within the specified version range.

Supported properties

The following properties are supported in expressions:

  • project.basedir: The project directory
  • project.rootDirectory: The root directory of the project
  • project.artifactId: The artifactId of the project
  • project.packaging: The packaging of the project
  • user properties
  • system properties (including environment variables prefixed with env.)

Examples

  • JDK version range: inrange(${java.version}, '[11,)') (JDK 11 or higher)
  • OS check: ${os.name} == 'windows'
  • File existence: exists('${project.basedir}/src/**/*.xsd')
  • Property check: ${my.property} != 'some-value'
  • Regex matching: matches(${os.version}, '.*aws')
  • Complex condition: ${os.name} == 'windows' && ${os.arch} != 'amd64' && inrange(${os.version}, '[10,)')
  • String length check: length(${user.name}) > 5
  • Substring with version: substring(${java.version}, 0, 3) == '1.8'
  • Using indexOf: indexOf(${java.version}, '-') > 0
  • Conditional logic: if(contains(${java.version}, '-'), substring(${java.version}, 0, indexOf(${java.version}, '-')), ${java.version})

This flexible condition mechanism allows for more precise control over profile activation, enabling developers to create profiles that respond to a wide range of environmental factors and project states.


This will be triggered using a new profile activation in the 4.1.0 model:

<profile>
  <activation>
    <condition>inrange(${maven.version}, '[4,)')</condition>
  </activation>
</profile>

@gnodet gnodet force-pushed the condition-profile branch from 85ab0d0 to 3092419 Compare October 3, 2024 17:47
@gnodet gnodet changed the title Add a condition profile based on a simple expression [MNG-8286] Add a condition profile based on a simple expression Oct 3, 2024
@gnodet gnodet changed the title [MNG-8286] Add a condition profile based on a simple expression [MNG-8286] Add a condition profile based on a simple expressions Oct 3, 2024
@gnodet gnodet added this to the 4.0.0-beta-5 milestone Oct 3, 2024
@gnodet gnodet force-pushed the condition-profile branch from 3092419 to a594d85 Compare October 3, 2024 21:25
Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems overly complicated. Why not just defer to an external script/command? If the script/command exits normally, then the profile is activated. If it exits with an error code, then it isn't activated.

For example:

<profile>
  <id>thisIsActivated</id>
  <activation>
    <command>/bin/true</command>
  </activation>
</profile>
<profile>
  <id>thisIsNotActivated</id>
  <activation>
    <command>/bin/false</command>
  </activation>
</profile>

There's no need to implement something so overly complex that is required to be maintained by the Maven developers, when you can simply defer to an external process that will be far more flexible to handle all possible user scenarios, with very little maintenance effort from the Maven developers.

@gnodet gnodet force-pushed the condition-profile branch 2 times, most recently from 6682190 to 362d9f1 Compare October 4, 2024 07:11
@gnodet
Copy link
Contributor Author

gnodet commented Oct 4, 2024

This seems overly complicated. Why not just defer to an external script/command? If the script/command exits normally, then the profile is activated. If it exits with an error code, then it isn't activated.

For example:

<profile>
  <id>thisIsActivated</id>
  <activation>
    <command>/bin/true</command>
  </activation>
</profile>
<profile>
  <id>thisIsNotActivated</id>
  <activation>
    <command>/bin/false</command>
  </activation>
</profile>

There's no need to implement something so overly complex that is required to be maintained by the Maven developers, when you can simply defer to an external process that will be far more flexible to handle all possible user scenarios, with very little maintenance effort from the Maven developers.

Because

  • a script is not portable across OS
  • it would not work when defined from external parents if your script is/was local to the project
  • advanced usage such as version range parsing would be very complex to implement with a shell script

Not all developers have full knowledge of shell scripting, so while I agree that [ "$os_name" = "windows" ] && [ "$os_arch" != "amd64" ] would work well, that's for sh only. On windows, you'd need to use either cmd with "%os_name%"=="windows" if not "%os_arch%"=="amd64" or powershell with $os_name -eq 'windows' -and $os_arch -ne 'amd64'. So that means at least 2 or three syntaxes for something so simple. Including the ability to specify multiple commands on when to use them. Also, after a bit of research, powershell does not use exit code automatically, so this will have to be:

  • sh -c [ "${os.name}" = "windows" ] && [ "${os.arch}" != "amd64" ]
  • powershell.exe -Command \"if (${os.name} -eq 'windows' -and ${os.arch} -ne 'amd64') { exit 0 } else { exit 1 }\"
    I'm not so sure about quotes on powershell, I never use it...
    In addition, this brings a lot of possible security problems if we start executing any kind of OS commands.

I agree there will be an additional maintenance cost, but we're in the process of removing lots of legacy code, so I think the introduction of a few hundreds of code lines are worth it.
I'd be fine with reusing a scripting library, but really, the parser is 400 lines... and we don't need anything complicated such as data structures, loops, etc...

@ctubbsii
Copy link
Member

ctubbsii commented Oct 4, 2024

Because

  • a script is not portable across OS

You can certainly write non-portable scripts/executables. But that's not what I suggested. I suggested leaving that up to the user. The user determines whether they are calling something portable or not. The fact that users could do non-portable things is irrelevant... they can do that today. Nothing about what I suggested forces them to be non-portable (assuming they even care about portability... which many don't). If they want to be portable, they can call portable external commands.

Here's a portable example using only Java: <command>java path/to/my/ProfileActivatorConditionA.java</command>

  • it would not work when defined from external parents if your script is/was local to the project

If the user wanted to write such a parent POM profile that is available to child projects, they could certainly write it in a way that is easily consumed by them. For example, <command>python -c 'import os; exit(0 if os.path.exists("pom.xml") else 1)'</command> only requires child projects have python on their path. They could even do: <command>mvn somepluginactivator:activate-conditionally</command>, which would be very portable and very consumable by child projects. I don't imagine very many projects would have profiles defined in the parent POM that would be activated by child projects, in this way, but it's certainly possible in my proposal.

  • advanced usage such as version range parsing would be very complex to implement with a shell script

A script could be written in any language, and an executable file could even be compiled code. I'm very skeptical of the idea that Maven, having invented a new, limited, domain-specific language, is going to be better for complex tasks than all the programming languages available to users already. In fact, I would probably bet that for any given expression you can provide, I could find another language out there that could perform the same task better. It's extremely unlikely that this new language is optimal for any particular use case.

Not all developers have full knowledge of shell scripting

Not all developers have full knowledge of this new language either, so I'm not sure what point is being made here. This isn't a prerequisite for either the proposed expression language, or for my suggestion. And in any case, I reject the premise... my suggestion was not to support only shell scripts... but any external executable. It isn't necessary to know any scripting to run an external command that checks a condition. It's only necessary to know the language that command was written in if you are the one writing it... but it's not necessary for any caller to know it at all.

In addition, this brings a lot of possible security problems if we start executing any kind of OS commands.

No, you'd only be executing the command configured by the developer to be executed, when those commands have permission to be executed within the user's security context... just like maven-jar-plugin effectively executes the jar command, the maven-compiler-plugin effectively executes javac, the maven-gpg-plugin executes gpg, and exec-maven-plugin executes the commands it is configured to execute, assuming those commands are available and have permission to be executed by the user. It is not a security concern to execute a command that was intended to be executed by the developer and explicitly configured to do so. If you think there are security problems with this, I'd like to understand more about the kind of problem you're envisioning.

I agree there will be an additional maintenance cost, but we're in the process of removing lots of legacy code, so I think the introduction of a few hundreds of code lines are worth it.

Eating an apple two days in a row is healthier than eating an apple one day and eating a whole cake the next. Adding unnecessary complexity is not justified merely because other complexity was reduced elsewhere. I think the merits of each new bit of complexity should be considered on their own, because every bit of complexity eliminated (or avoided) has merit.

However, my suggestion isn't just about reduction of complexity. If it were just a disagreement over whether the complexity is worth the added value from the feature, I wouldn't have said anything... because that's a value judgment and values are subjective. The only reason I commented was because I think that complex feature actually satisfies fewer use cases than what I'm suggesting. And, it feels like you're paying for the cost of complexity to get a snippet, when you could have less complexity and get the whole thing. Why pay for the movie trailer when it's cheaper to watch the whole movie with popcorn and snacks?

I'd be fine with reusing a scripting library, but really, the parser is 400 lines... and we don't need anything complicated such as data structures, loops, etc...

Who says you don't need those things? I think that's a bit short-sighted. It seems like you are simultaneously arguing that users have need for "advanced usage" that would be too complicated to implement in an external scripting language, but those usages are still simple enough that they don't need basic elements of a programming language like loops and data structures (and perhaps variables?). I don't see how you can argue both positions simultaneously. I think if users have simple usages, they can call simple external commands, and if they have advanced usages, they can call advanced external commands. But, I'd argue leaving it up to the user, and keeping Maven code and behavior simple.

Sorry for the length and if this response is unpolished... I'm responding late and am tired 😺

@gnodet
Copy link
Contributor Author

gnodet commented Oct 4, 2024

Because

  • a script is not portable across OS

You can certainly write non-portable scripts/executables. But that's not what I suggested. I suggested leaving that up to the user. The user determines whether they are calling something portable or not. The fact that users could do non-portable things is irrelevant... they can do that today. Nothing about what I suggested forces them to be non-portable (assuming they even care about portability... which many don't). If they want to be portable, they can call portable external commands.

Here's a portable example using only Java: <command>java path/to/my/ProfileActivatorConditionA.java</command>

  • it would not work when defined from external parents if your script is/was local to the project

If the user wanted to write such a parent POM profile that is available to child projects, they could certainly write it in a way that is easily consumed by them. For example, <command>python -c 'import os; exit(0 if os.path.exists("pom.xml") else 1)'</command> only requires child projects have python on their path. They could even do: <command>mvn somepluginactivator:activate-conditionally</command>, which would be very portable and very consumable by child projects. I don't imagine very many projects would have profiles defined in the parent POM that would be activated by child projects, in this way, but it's certainly possible in my proposal.

  • advanced usage such as version range parsing would be very complex to implement with a shell script

A script could be written in any language, and an executable file could even be compiled code. I'm very skeptical of the idea that Maven, having invented a new, limited, domain-specific language, is going to be better for complex tasks than all the programming languages available to users already. In fact, I would probably bet that for any given expression you can provide, I could find another language out there that could perform the same task better. It's extremely unlikely that this new language is optimal for any particular use case.

Not all developers have full knowledge of shell scripting

Not all developers have full knowledge of this new language either, so I'm not sure what point is being made here. This isn't a prerequisite for either the proposed expression language, or for my suggestion. And in any case, I reject the premise... my suggestion was not to support only shell scripts... but any external executable. It isn't necessary to know any scripting to run an external command that checks a condition. It's only necessary to know the language that command was written in if you are the one writing it... but it's not necessary for any caller to know it at all.

In addition, this brings a lot of possible security problems if we start executing any kind of OS commands.

No, you'd only be executing the command configured by the developer to be executed, when those commands have permission to be executed within the user's security context... just like maven-jar-plugin effectively executes the jar command, the maven-compiler-plugin effectively executes javac, the maven-gpg-plugin executes gpg, and exec-maven-plugin executes the commands it is configured to execute, assuming those commands are available and have permission to be executed by the user. It is not a security concern to execute a command that was intended to be executed by the developer and explicitly configured to do so. If you think there are security problems with this, I'd like to understand more about the kind of problem you're envisioning.

I agree there will be an additional maintenance cost, but we're in the process of removing lots of legacy code, so I think the introduction of a few hundreds of code lines are worth it.

Eating an apple two days in a row is healthier than eating an apple one day and eating a whole cake the next. Adding unnecessary complexity is not justified merely because other complexity was reduced elsewhere. I think the merits of each new bit of complexity should be considered on their own, because every bit of complexity eliminated (or avoided) has merit.

However, my suggestion isn't just about reduction of complexity. If it were just a disagreement over whether the complexity is worth the added value from the feature, I wouldn't have said anything... because that's a value judgment and values are subjective. The only reason I commented was because I think that complex feature actually satisfies fewer use cases than what I'm suggesting. And, it feels like you're paying for the cost of complexity to get a snippet, when you could have less complexity and get the whole thing. Why pay for the movie trailer when it's cheaper to watch the whole movie with popcorn and snacks?

I'd be fine with reusing a scripting library, but really, the parser is 400 lines... and we don't need anything complicated such as data structures, loops, etc...

Who says you don't need those things? I think that's a bit short-sighted. It seems like you are simultaneously arguing that users have need for "advanced usage" that would be too complicated to implement in an external scripting language, but those usages are still simple enough that they don't need basic elements of a programming language like loops and data structures (and perhaps variables?). I don't see how you can argue both positions simultaneously. I think if users have simple usages, they can call simple external commands, and if they have advanced usages, they can call advanced external commands. But, I'd argue leaving it up to the user, and keeping Maven code and behavior simple.

Sorry for the length and if this response is unpolished... I'm responding late and am tired 😺

External commands are clearly a no-go. Profile activations need to be parsed in an independent way, outside the build environment, because they end up in Maven central parent poms. I fully disagree with your argument that it "only requires child projects have python on their path". We can't make it pluggable for the exact same reason (i.e. they cannot be extended by extensions as it would break if used in a different context). If we add anything in profile activation, it has to be available for everyone in all contexts. So no external requirements imho.

# Conflicts:
#	compat/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/MavenRepositorySystemSupplier.java
#	impl/maven-impl/src/main/java/org/apache/maven/internal/impl/DefaultSettingsBuilder.java
#	impl/maven-impl/src/main/java/org/apache/maven/internal/impl/model/DefaultModelBuilder.java
#	impl/maven-impl/src/main/java/org/apache/maven/internal/impl/model/profile/ConditionFunctions.java
#	impl/maven-impl/src/main/java/org/apache/maven/internal/impl/model/profile/ConditionParser.java
#	impl/maven-impl/src/main/java/org/apache/maven/internal/impl/model/profile/ConditionProfileActivator.java
@gnodet gnodet force-pushed the condition-profile branch from 8582bcd to 3b24de6 Compare October 25, 2024 19:32
@gnodet
Copy link
Contributor Author

gnodet commented Nov 4, 2024

We may need to take into accounts https://issues.apache.org/jira/browse/MNG-6943 and somehow make the selected toolchain available for additional checks.

# Conflicts:
#	impl/maven-impl/src/main/java/org/apache/maven/internal/impl/DefaultSettingsBuilder.java
@gnodet gnodet removed this from the 4.1.0 milestone Nov 20, 2024
@gnodet gnodet added this to the 4.0.0-rc-1 milestone Nov 20, 2024
@gnodet gnodet merged commit 986f596 into apache:master Nov 20, 2024
13 checks passed
@gnodet
Copy link
Contributor Author

gnodet commented Nov 20, 2024

We may need to take into accounts https://issues.apache.org/jira/browse/MNG-6943 and somehow make the selected toolchain available for additional checks.

Profiles are activated quite early, while toolchains are selected later in the process. If we want to support these use cases, we need to have a more declarative way to select a JDK, so that we can know during profile activation, which JDK will be used.

@gnodet gnodet deleted the condition-profile branch December 13, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants